-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Colorize colcon output #195
base: master
Are you sure you want to change the base?
Conversation
Well, After seeing failed checks I realized that the solution is only good for Linux, and I didn't test it on Windows at all. I failed to find some instructions on testing these things before actually submitting a pull request, and I will be glad if someone will point out what can I do to complete the solution and actually have this feature implemented in |
Thank you for working on this enhancement.
Yeah, any patch should work across all supported platforms (as long as that is feasible).
That sounds right. The code style follows PEP 8 and uses
Beside building your changes in the |
The way catkin_tools handles this is that the actual logic outputs bare strings, and then there's a translation process (similar to translating for another human language) which swaps those out for colorized versions based verb-supplied lookup maps: https://github.com/catkin/catkin_tools/search?q=_color_translation_map |
@yossioo Are you interested to iterate on your PR? |
Definitely! I am eager to see colorful Do you think I should stick with this approach but add some system checks to constrain solution to be Ubuntu only? Update: |
Let me try to summarize my perspective:
With this wish list I currently don't see how this could be implemented. And if not all goals can be satisfied I am hesitant to decide which one should be / needs to be dropped. Any suggestions are very welcome. |
The interception-based approach (like how Or just use the logging framework's LoggerAdapter to have different logger objects for different contexts: https://docs.python.org/3/library/logging.html#loggeradapter-objects Even with zero additional effort to inject color into existing outputs, this would still allow adding some small niceties like coloring a command's stderr differently from something that was logged directly by a colcon module or plugin. |
Can you sketch a snippet how you imagine the invocation to look like?
Currently the messages which should be colorized are not using the
How would you distinguish these two cases - a commands output printed in one place and a random status message printed somewhere else? |
I'm surprised you're not using I don't have much familiarity with the colcon codebase, but from a brief look, it seems that the start/end messages are emitted with {{print()}} here, one of which uses {{file=}} to direct them to stdout or stderr as appropriate: https://github.com/colcon/colcon-core/blob/master/colcon_core/event_handler/console_start_end.py This would be easy to handle with
And then later on, send messages to it with Handler instances determine which Formatter is used for a particular message. So the colorizing Formatter could be one of several options, with rules that direct particular logger names to coloring or non-coloring formatter. The command output would just have to be sent using a differently-named logger, something like Anyhow, if this is of interest I could look at preparing a more extensive example (eg, porting enough of colcon to it to more fully demonstrate the principle), but that would likely be several weeks away. |
colcon-core/colcon_core/logging.py Line 16 in e45426e
The "normal" output is using just plain How do you imagine the custom colorization to come in then? Would some Python code register a custom formatter for a known
The log files already have timestamps on each line. |
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
==========================================
+ Coverage 78.09% 79.89% +1.79%
==========================================
Files 53 55 +2
Lines 3059 3173 +114
Branches 512 525 +13
==========================================
+ Hits 2389 2535 +146
+ Misses 642 595 -47
- Partials 28 43 +15
Continue to review full report at Codecov.
|
Hey, I just looked at Dirk's analysis of the problem above and thought I could propose a solution: We could intercept all output and search for color format strings (something like So far so good (I guess this was also @dirk-thomas and @mikepurvis idea). The problem is where to put this interceptor, and I think we need to put it in To answer Dirk's question, I would weaken his first point. Colcon is highly modular, yes, but at least a default We could also have more semantic annotations like Thoughts? |
If every code line which prints a message already includes for colorization information in the message, what is the benefit of using a custom format and not simply ASCII control characters? The main question is: do we really want all code to be aware of this aspect and include colorization information in each message or do we want the colorization to come in orthogonality as it is done in
The fact that |
But I see your point, it's probably a simple viable solution to constrain support to ANSI codes (e.g. 8 colors) and to somehow hook a filter for them into |
E.g. [colorama[(https://pypi.org/project/colorama/) provides a way to make this work cross platform.
Commonly human readable variables containing these ASCII codes are being used.
Can you elaborate why you think that is the case? |
Not OP, but I assume the fact that ANSI codes may have different lengths depending on content so AFAIK you have to understand the ANSI codes to be able to remove them. EDIT: But I presume there should be many libraries for that |
I think this is superseded by #487 |
Greetings.
I propose a colorized version of
colcon
's output.Tested on Ubuntu 18.04, with ROS2 Dashing.
Screenshot is here.